use /map: linker flag to avoid running a binary to capture the hash#3133
use /map: linker flag to avoid running a binary to capture the hash#3133yoavwizstein wants to merge 7 commits into
Conversation
yoavwizstein
had a problem deploying
to
manual-approval
GitHub Actions
Error
yoavwizstein
had a problem deploying
to
manual-approval
GitHub Actions
Error
yoavwizstein
had a problem deploying
to
manual-approval
GitHub Actions
Error
yoavwizstein
had a problem deploying
to
manual-approval
GitHub Actions
Error
yoavwizstein
had a problem deploying
to
manual-approval
GitHub Actions
Error
yoavwizstein
had a problem deploying
to
manual-approval
GitHub Actions
Error
There was a problem hiding this comment.
Thanks for this PR! The approach of using the /MAP: linker flag to avoid running a binary during build aligns with how Linux and macOS already work.
The previous capture_hash approach had a nice side-effect: by running fips_empty_main.exe and observing the integrity check fail (with the placeholder hash), it implicitly proved the integrity check machinery was wired up and functional. With the new static-analysis approach, that implicit verification goes away.
To compensate, I've pushed a commit on top of yours that adds FIPS integrity validation to the Windows CI. It does a couple of things:
-
Extends
break-hash.gowith PE support -- this existing tool could only corrupt ELF binaries. It now accepts a-mapflag and uses the same map file +debug/peapproach your PR adds toinject_hash.goto locate and corrupt the FIPS module in a Windows DLL. -
Added a
fips_build_and_testsubroutine torun_windows_tests.bat-- after the normal build and test pass, it runs a negative test: corruptscrypto.dllviabreak-hash.go, runstest_fips.exe, and verifies the process aborts. This proves the integrity check actually runs on DLL load and can detect tampering.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3133 +/- ##
=======================================
Coverage 78.12% 78.12%
=======================================
Files 689 689
Lines 123214 123214
Branches 17137 17136 -1
=======================================
+ Hits 96257 96266 +9
+ Misses 26047 26039 -8
+ Partials 910 909 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Haven't looked at the full implementation yet. When inspecting some of the failing CI dimensions, removal of |
justsmth
force-pushed
the
linux-windows-cross-rebased
branch
from
c37175b to
ac27524
Compare
justsmth
had a problem deploying
to
manual-approval
GitHub Actions
Error
justsmth
had a problem deploying
to
manual-approval
GitHub Actions
Error
justsmth
had a problem deploying
to
manual-approval
GitHub Actions
Error
justsmth
had a problem deploying
to
manual-approval
GitHub Actions
Error
justsmth
force-pushed
the
linux-windows-cross-rebased
branch
from
ff4483b to
0bb36b2
Compare
- aws-lc-rs.yml: rewrite stale Wine-setup comment. The build no longer
runs fips_empty_main.exe; Wine is only needed for the downstream
FIPS sanity test step that loads the cross-built DLL.
- run_windows_tests.bat: restore crypto.dll from backup before checking
the negative-test exit code, so a failure here does not leave a
corrupted DLL behind for subsequent local invocations.
- inject_hash.go (doWindows): require both BORINGSSL_bcm_rodata_{start,end}
markers unconditionally. Windows FIPS is always a shared-library build
and the runtime hashes rodata in that configuration; silently skipping
rodata here would produce a hash that disagrees with the runtime.
- fipscommon/pe.go: add trailing newline at EOF.
ParseMapFile: - Anchor parsing to the Publics by Value column header so a same-named entry in the Static symbols section can't shadow or collide with a public BORINGSSL_bcm_* marker. - Fail fast with a descriptive error if the map file does not contain a Publics by Value section, instead of producing a downstream 'symbol not found in map file' error. - Expand the function doc with the expected map-file grammar. break-hash.go: - Add a comment on doPE explaining why it does not need the 256-byte prefix uniqueness check that doELF performs: the linker map plus the PE section table already yield an exact on-disk offset for BORINGSSL_bcm_text_start, so the ELF-era workaround is unnecessary.
justsmth
force-pushed
the
linux-windows-cross-rebased
branch
from
45d7a21 to
1dfd266
Compare
4 participants
Issues:
Resolves #3021
Description of changes:
Replaces the Windows FIPS hash-injection mechanism. Previously, the build linked and ran
fips_empty_main.exeto trigger the integrity self-test, captured the expected hash from its stderr, then patched it intocrypto.dll. This required a Windows (or Wine) execution environment at build time, blocking cross-compilation.Now Windows uses the same approach as Linux/macOS: the linker produces a map file (
/MAP:), andinject_hash.go -windowsparses it alongside the PE to locate the FIPS module boundaries, compute the hash, and patch the DLL directly. No binary execution needed.Call-outs:
fips_empty_main.candcapture_hash/capture_hash.goare deleted entirely.break-hash.gois extended to handle PE via the newfipscommonpackage (shared withinject_hash.go)./Fo:→/Fochange infipsmodule/CMakeLists.txtdrops the colon separator for compatibility withclang-cl.lib.exeinvocation now uses a response file (bcm_objects.rsp) to avoid command-line length limits.lib.exediscovery usesfind_programinstead of assumingCMAKE_ARor a hardcoded path relative to the linker, improving robustness with clang-cl toolchains.Testing:
The Windows CI (
run_windows_tests.bat) now runs a dedicatedfips_build_and_testroutine for FIPS dimensions that performs both a positive test (test_fips.exeloadscrypto.dlland passes the integrity check + KATs) and a negative test (corrupts the FIPS text region viabreak-hash.go -map, then confirmstest_fips.exeaborts). The cross-compilation path is exercised in theaws-lc-rs.ymlworkflow which builds withcargo xwinand runs the FIPS sanity test under Wine.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.